Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(s3): export bucket websiteURL (#1521) #1522

Closed
wants to merge 26 commits into from
Closed

feat(s3): export bucket websiteURL (#1521) #1522

wants to merge 26 commits into from

Conversation

AlexChesters
Copy link
Contributor

This exposes the WebsiteURL property (documented in the relevant CloudFormation template).


Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • [ not needed ] Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@AlexChesters AlexChesters requested a review from a team as a code owner January 11, 2019 14:49
@@ -199,6 +199,13 @@ export interface BucketImportProps {
* @default Inferred from bucket name
*/
bucketDomainName?: string;

/**
* The website URL of the bucket (if static web hosting is enabled).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how useful this description is, I tried a couple of different variations but couldn't come up with anything better than this.

Suggestions welcome!

@@ -465,6 +467,17 @@ export = {
"Export": {
"Name": "S1:MyBucketDomainNameF76B9A7A"
}
},
"MyBucketWebsiteURL9C222788": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the logical ID here (i.e. MyBucketWebsiteURL9C222788) from running the tests, watching them fail and observing the output.

I'm not sure if I was meant to have determined the output from somewhere else?

*
* @default Inferred from bucket name
*/
bucketWebsiteURL?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to bucketWebsiteUrl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between this and domainName? I also can't seem to find the actual implementation in Bucket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @eladb 😄

I've pushed the change to rename to bucketWebsiteUrl now.

Ah, I've just spotted that this actually will be identical to domainName. Good spot, I'll push a fix for that.

@AlexChesters
Copy link
Contributor Author

Hi @eladb,

Thanks again for the review. I've pushed the necessary changes now. I've determined the format of the website URL from the AWS docs.

Thanks!

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still can't find the implementation in "normal" Bucket (not ImportedBucket)

@@ -620,6 +629,7 @@ export class Bucket extends BucketBase {
bucketArn: new cdk.Output(this, 'BucketArn', { value: this.bucketArn }).makeImportValue().toString(),
bucketName: new cdk.Output(this, 'BucketName', { value: this.bucketName }).makeImportValue().toString(),
bucketDomainName: new cdk.Output(this, 'DomainName', { value: this.domainName }).makeImportValue().toString(),
bucketWebsiteUrl: new cdk.Output(this, 'WebsiteURL', { value: this.bucketWebsiteUrl }).makeImportValue().toString()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if bucketWebsiteUrl is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in from a consumer's point of view? Currently it would just result in the property being undefined as you said.

I've had a go at creating separate test cases (for cases where a bucket does and doesn't have a website configuration) but I'm not 100% sure how to proceed. Looking at the other cases it seems the usual way of testing this stuff is to generate the CloudFormation and then assert against that. In this case, however, the value is just an Fn::GetAtt snippet:

'exports the WebsiteURL is website configuration is enabled'(test: Test) {
  const stack = new cdk.Stack();
  const bucket = new s3.Bucket(stack, 'Website', {
    bucketName: 'my-bucket',
    websiteIndexDocument: 'index.html'
  });
  test.deepEqual(cdk.resolve(bucket.bucketWebsiteUrl), 'my-bucket-<REGION>.s3-website.amazonaws.com');
  test.done();
}

The are two problems I've ran into when attempting to create test cases like the above:

  1. AssertionError: { 'Fn::GetAtt': [ 'Website32962D0B', 'WebsiteURL' ] } deepEqual 'my-bucket-<REGION>.s3-website.amazonaws.com'
    1. How can I compare against the actual value as opposed to an Fn::GetAtt snippet? (I assumed calling cdk.resolve would have solved this
    2. How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it would just result in the property being undefined as you said.

why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?

How can I compare against the actual value as opposed to an Fn::GetAtt snippet? (I assumed calling cdk.resolve would have solved this

You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).

How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases

What I usually do is print out the output of cdk.resolve, inspect it to make sure it's what I expect, and then use it as my test expectation:

Copy link
Contributor Author

@AlexChesters AlexChesters Jan 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?

Ah, I thought you were asking what would happen if the user hadn't set up website hosting for the bucket. In the case where they have set up website hosting for the bucket, bucketWebsiteUrl should never be undefined. Is there something you've spotted that might result it in being undefined?

You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).
What I usually do is print out the output of cdk.resolve, inspect it to make sure it's what I expect, and then use it as my test expectation:

Gotcha, I'll give this a go. Thanks!

@@ -985,4 +997,8 @@ class ImportedBucket extends BucketBase {
private generateDomainName() {
return `${this.bucketName}.s3.amazonaws.com`;
}

private generateBucketWebsiteUrl() {
return `${this.bucketName}.s3-website-${new cdk.AwsRegion()}.amazonaws.com`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this look in a different partition? (i.e. china)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a.. good question. I wasn't aware (until I found these docs) that the domain name was different in regions such as China.

Happy to take a look at making sure this works in regions such as China, would you want me to take a look at doing the same for the generateDomainName() function above (as that appears to have the same problem), or leave that for a separate issue and PR?

@AlexChesters
Copy link
Contributor Author

Still can't find the implementation in "normal" Bucket (not ImportedBucket)

@eladb I think I see what you mean, but I'm struggling to see exactly where I need to define this in normal Bucket. Are you able to point me in the general direction? Perhaps where some of the other properties (e.g. domainName) are implemented?

@eladb
Copy link
Contributor

eladb commented Jan 14, 2019

Are you able to point me in the general direction?

Sure, something around here

@AlexChesters
Copy link
Contributor Author

Sure, something around here

Thanks, I had done something like that here. I couldn't see anywhere else where resource.bucketDualStackDomainName was implemented so I just copied the line below. Is that wrong?

@eladb
Copy link
Contributor

eladb commented Jan 14, 2019

Thanks, I had done something like

Oh right. sorry, my bad. I missed this. Looks good.

@AlexChesters
Copy link
Contributor Author

Oh right. sorry, my bad. I missed this. Looks good.

Not a problem 😄

I've added a unit test, let me know your thoughts on that?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 14, 2019

The automatic test failed.

@AlexChesters
Copy link
Contributor Author

@rix0rrr are you able to shed any light on that test failure? I've ran ./install.sh && ./build.sh on my machine (which, as far as I can see, is what Travis is running) and didn't have any problems.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 14, 2019

I think this is due to some refactorings we've done on master.

cdk.resolve() for example needs to change to this.node.resolve(). You should probably merge from master and try again.

Sorry for the hassle.

@AlexChesters
Copy link
Contributor Author

I think this is due to some refactorings we've done on master.
cdk.resolve() for example needs to change to this.node.resolve(). You should probably merge from master and try again.
Sorry for the hassle.

Ah, of course. Don't apologise, my fault entirely! I'll merge master into my branch and resolve the issues. Apologies!

workeitel and others added 9 commits January 14, 2019 14:43
Small change to enable tagging KMS keys.
The `cdk diff` command intentionally returns non-zero when a difference is found,
similar to how the POSIX `diff` command behaves (returning 0 when no difference is
found, 1 when some difference is found, and >1 when an error occurred). This
behavior was however undocumented, possibly leading to user confusion.

This change adds a note in `cdk --help` that mentions the exit code 1 will be used
when a difference is found.

Fixes #1440
Since #973, the renames of CFN properties from "name" to "xxxName" were
removed. But the ELBv2 library still used `loadBalancerName`. The reason
this was not discovered was because we are splatting `additionalProps` of
type `any`, and this caused the type checker to stop verifying property
names.

Fixes #1481
Use a unified diff format to render differences in arbitrary values,
making it easier to understand what is changing in possibly large JSON
structures, for example.

The number of context lines used when rendering the JSON differences
can be customized using the `--context-lines` option of `cdk diff`, 
which has a default value of `3`.
Adds support for CNAME records via the `CnameRecord` construct.

Misc:

- `TXTRecord` was renamed to `TxtRecord`.
- `hostedZoneNameServers` attribute added to IHostedZone
- Made `HostedZone` a concrete (non-abstract) class so it will be
  compatible with how CloudFormation represents this resource,
  but left PublicHostedZone and PrivateHostedZone to allow
  a more strongly-typed experience if you like.

Credits for original PR (closes #1420): @MikeBild

BREAKING CHANGE: The `route53.TXTRecord` class was renamed to `route53.TxtRecord`.
Now that we can represent attributes as native strings and string lists,
this covers the majority of resource attributes in CloudFormation.

This change:

* String attributes are represented as `string` (like before).
* String list attribute are now represented as `string[]`.
* Any other attribute types are represented as `cdk.Token`.

Attributes that are represented as Tokens as of this change:

* amazonmq has a bunch of `Integer` attributes (will be solved by #1455)
* iot1click has a bunch of `Boolean` attributes
* cloudformation has a JSON attribute
* That's all.

A few improvements to cfn2ts:

* Auto-detect cfn2ts scope from package.json so it is more self-contained
  and doesn't rely on cdk-build-tools to run.
* Added a "cfn2ts" npm script to all modules so it is now possible
  to regenerate all L1 via "lerna run cfn2ts".
* Removed the premature optimization for avoiding code regeneration
  (it saved about 0.5ms).

Fixes #1406

BREAKING CHANGE: any `CfnXxx` resource attributes that represented a list of strings are now typed as `string[]`s (via #1144). Attributes that represent strings, are still typed as `string` (#712) and all other attribute types are represented as `cdk.Token`.
Elad Ben-Israel and others added 12 commits January 14, 2019 14:43
BREAKING CHANGE: the deprecated `cloudformation.XxxResource` classes have
been removed. Use the `CfnXxx` classes instead.
Change `conditionXx` methods to accept an interface `IConditionExpression`
instead of concrete class and implement this interface by the `Condition`
construct.

This enables chaining conditions like so:

    const cond1, cond2, cond, cond4 = new cdk.Condition(...)
    Fn.conditionOr(cond1, cond2, Fn.conditionAnd(cond3, cond4))

Fixes #1457
AWS Step Functions released support for 'Parameters' option in the input definition.

Closes #1480
It is now no longer necessary to use `export()` and `import()` when
sharing constructs between two `Stacks` inside the same CDK app.
Instead, objects defined in one stack can be used directly in another
stack.

The CDK will detect when an attribute (such as an ARN, ID or URL) of
such an object is used in a different stack, and will automatically
create the required `Export` in the producing stack and insert the
corresponding `Fn::ImportValue` in the consuming stack.

BREAKING CHANGE: if you are using `export()` and `import()` to share
constructs between stacks, you can stop doing that, instead of
`FooImportProps` accept an `IFoo` directly on the consuming stack,
and use that object as usual. `ArnUtils.fromComponents()` and
`ArnUtils.parse()` have been moved onto `Stack`. All CloudFormation
pseudo-parameter (such as `AWS::AccountId` etc) are now also
accessible via `Stack`, as `stack.accountId` etc. `resolve()` has
been moved to `this.node.resolve()`. `CloudFormationJSON.stringify()`
has been moved to `this.node.stringifyJson()`. `validate()` now
should be `protected`.

Fixes #1324.
In order to allow passing in an imported role to a Lambda/Alias,
we must use `IRole` instead of `Role`.
The "toCloudFormation" method of generated CFN resources invoke
"resolve" so that any lazy tokens are evaluated. This escaped the
global settings set by `findTokens` which collect tokens so they
can be reported as references by `StackElement.prepare`.

To solve this, findTokens now accepts a function instead of an object
and basically just wraps it's invocation with settings that will collect
all tokens resolved within that scope. Not an ideal solution but
simple enough.

This was not discovered because we didn't have any tests that validated
the behavior of the generated CFN resources (they are not accessible
from the @aws-cdk/cdk library). We add a unit test in the IAM library to cover this use case.
Two improvements to stack dependency handling in the toolkit:

* Destroy stacks in reverse order. This is necessary to properly dispose of stacks that
  use exports and `Fn::ImportValue`. Fixes #1508.
* Automatically include stacks that have a dependency relationship
  with the requested stacks, unless `--exclusively` (`-e`) is
  passed on the command line. Fixes #1505.
* v0.22.0

See CHANGELOG
add packages.txt to github pages
Jetbrains IDEs will index the files in a project to provide intelligent suggestions to users. However, this project uses lerna and has cyclical symlinks inside node_module directories. The IDE will index until it runs out of memory following these links. Given the size of this project, doing the exclusions by hand is infeasable every time one does a git clean. This script will modify the .iml file directly to add the appropriate exclusions.

* The JetBrains IDEs exclusion list assumed your iml file was named aws-cdk. It now uses the root directory's name instead.
* IntelliJ stores the iml file at root. The script now accounts for that difference.
* For uncommon setups, the script can take an optional path arguement to opt-out of conventional file discovery, and be explicit.
@AlexChesters
Copy link
Contributor Author

ugh. my poor git skills have made this PR a bit unwieldy. I'm going to close this for now and open a new PR with a cleaner history.

Apologies @eladb, @rix0rrr and everyone else who has now been pinged about this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.